I pretty sure there is better place for signal freezing than first write attempt.

Actually I was quite surprised with the whole concept of freeze(). It's very tricky! Do you know why it's necessary at all ?
The only reason I can think of is that the author wanted to prevent modifications to list of handlers while processing send().

Still, it's possible that we will decide to remove this Signal altogether, but if we don't, what do you think is the correct place to call freeze() ?

• I can't do this in __init__ because I want to add handlers later
• I can't delegate this responsibility to the user of the class, because this introduces a braking change where a code which works now like:

  writer = StreamWriter(...)
  await writer.write(...)
  

  will start raising RuntimeError

Don't touch if you don't change the logic.
If you want update code style -- do it in a separate PR.
For this particular line I pretty happy with status quo.

That's wasn't me, it's my emacs ;) I will revert this change

Fixed

Also I fixed setup.cfg so that this line is not altered by anyone else who uses autopep8

Cool

Spawning a new task is not what we need.
Technically .write is a coroutine, we need convert it to genuine async function.
Maybe I'll do it very soon in separate PR.

Are you sure we don't want this in another task ? Whatever handler does asynchronously will slow down writing the response.

Also .write isn't actually coroutine, because it's not called like:

                for chunk in self.body:
                    writer.write(chunk)

(without await) The couroutine returned is never triggered.
I guess I would have to track all the places there StreamWriter.write is used and add await in there. That's not undoable, but it's rather big refactor.

Yes, I'm sure. Spawning a task without waiting for result has an ugly smell.
More important spawning a task may reorder received signals, e.g. user may get request chunk after request finished.

.write should be async method. I'm working on this refactoring (required for #2698 anyway).
I suggest waiting for my job done.

Got it, I will wait until the refactoring is merged.

Not sure if we need a signal here.
It looks like signal concept overusage: you are creating on_chunk_received signal with the only usage by aiohttp internals itself. It is not very effective, signal subscription is literally a waste of time.
I suggest passing traces directly to response object.

Here the approach with passing traces will actually work quite well. I've introduced the signal to make it symetrical to the situation in request. I will update this to pass traces and remove the signal

Again pass traces instead of creating a new internal-only signal.

Well, please consider that StreamWriter is used both by the http client and server. If I pass the traces in __init__() I will have to make something like:

def write( ):
    for trace in self.traces:
        await trace.sent_request_chunk_sent(...)
    ...

Now if you consider that code in the context of usage by http server, it clearly stops making sense....
This is why I introduced a signal and names it on_chunk_sent. Without specifying if it's http request it's sending or http response (as a server).

Let's avoid signals for internal-only usage.
Here we have two options: pass a callback for sending signals or derive a ClientStreamWriter class. Personally I prefer the second approach.

I will remove the Signal in favor of accepting an optional callable to be used as a callback.
I prefer not to create a child class for this - too many layers of inheritance hurts code readability.

Ok

Convert the function into ClientRequest's method. No need to create a nested function on every HTTP request.

done

Please do check self._on_chunk_sent is not None. It is more idiomatic.

changed

Do you really need to touch all these lines?
Appending two classes to the end of sequence should be enough.

Ok, reverted and added new classes at the end

Please also upgrade flow diagrams at the beginning of file.

done. Here I added new signals to description. Please let me know if you meant something more elaborate.

Let's not use mutables as defaults. These may cause some nasty bugs we could easily avoid.

true, changed to None

Why in ClientRequest traces is public attribute while in ClientResponse it's private?

I can answer: ClientResponse is a public class visible by user.
The attr should be private.
About request I don't care.

Ok, fine for me.

Agreed. I switched now to _traces in both classes

Same defaults story.

also changed to None

Please rename got_response and send_request nodes to be close to actual signal names. Maybe that long names could be shortened on diagram -- be creative.

Also I like to see arrows what explicitly points that chunk_sent and chunk_received events can come more than once.

I'm in doubt that we should ignore these. Any reasons why to disable them globally?

Well, I'm not in place to judge this. I did it to be able to save a file which has 64*1000 literal without spaces around *. Otherwise autopep just fixes it for me.

That autopep suggestion looks correct. You may also write 64000 instead without any loss in readability.

I'm with you. @asvetlov specifically asked me to keep it as was.

For me the strange thing is successful passing flake8 checks without the ignore setting.
I'm totally fine with 64000 or even better 0x10000. Pretty sure it should be 64KiB instead of 64kB

Please drop the change. Feel free to replace the limit with 0x10000 if needed -- I'm +-0 for it.

done

Move the line after the signal description, below params is :class:aiohttp.TraceRequestChunkSentParams instance. line.

This is our style guide for aiohttp documentation

Please add versionadded here as well

done